Ignore ZST offsets when deciding whether to use Scalar/ScalarPair layout #73453

erikdesjardins · 2020-06-17T19:21:42Z

This is important because Scalar/ScalarPair layout previously would not be used if any ZST had nonzero offset.
For example, before this change, only ((), u128) would be laid out like u128, not (u128, ()).

Fixes #63244

rust-highfive · 2020-06-17T19:21:45Z

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @petrochenkov (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

Please see the contribution instructions for more information.

src/librustc_middle/ty/layout.rs

src/librustc_mir/transform/const_prop.rs

petrochenkov · 2020-06-17T22:08:03Z

AFAIR, this restriction was introduced to be able to do DST coercions, but I'm not sure whether this applies to tuples.

r? @RalfJung

erikdesjardins · 2020-06-17T22:55:17Z

Yep, this PR miscompiles the following code :(

#![feature(unsized_tuple_coercion)]

pub type Siz = (u8, [u32; 2]);
pub type Un = (u8, [u32]);

pub fn coercion(x: &Siz) -> &Un {
    x
}

LLVM IR

define { { [0 x i8], i8, [3 x i8], [0 x i32] }*, i64 } @_ZN4test8coercion17h76ca7bfe6ae8dcf2E({ [0 x i32], [2 x i32], [0 x i8], i8, [3 x i8] }* noalias readonly align 4 dereferenceable(12) %x) unnamed_addr #0 {
start:
  %0 = bitcast { [0 x i32], [2 x i32], [0 x i8], i8, [3 x i8] }* %x to { [0 x i8], i8, [3 x i8], [0 x i32] }*
  %1 = insertvalue { { [0 x i8], i8, [3 x i8], [0 x i32] }*, i64 } undef, { [0 x i8], i8, [3 x i8], [0 x i32] }* %0, 0
  %2 = insertvalue { { [0 x i8], i8, [3 x i8], [0 x i32] }*, i64 } %1, i64 2, 1
  ret { { [0 x i8], i8, [3 x i8], [0 x i32] }*, i64 } %2
}

Should I: limit this to types that don't implement Unsize<T> (if that's possible and reasonable to do for layout); or limit this to ZSTs in the last field; or give up on making this change and just add the above code as a test?

RalfJung · 2020-06-18T07:04:31Z

This is way outside my league I'm afraid.^^ But AFAIK it is indeed impossible to reorder the last field due to unsizing coercions.

just add the above code as a test?

That would definitely be a good idea. I cannot give any advice on the other alternatives, sorry.

Cc @eddyb (but they are very busy recently)... not sure whom else to ping? Cc @nikomatsakis

nikomatsakis · 2020-06-18T15:54:35Z

r? @nikomatsakis -- I'll take a look and try to give intelligent feedback :)

erikdesjardins

Changed to instead allow reordering of the last field of a MaybeUnsized struct iff it's a ZST, which I think is most likely to be viable (if anything is).

erikdesjardins · 2020-06-28T19:49:03Z

src/librustc_middle/ty/layout.rs

+                    // This will reorder the last field if it is a ZST, which is okay because
+                    // there's nothing in memory that could be accessed through an unsized type.


I'm not sure if this is actually okay.

nikomatsakis · 2020-07-01T21:35:14Z

Well, the logic makes some sense to me, at least, but I'd like to hear what @eddyb thinks, as they definitely know this code best (I'm not all that familiar with it, to be honest).

I guess the "other way around" here would be not to reorder fields but to make the checks and code that are failing with the current ordering more capable?

I guess in any case that the logic is that we can 'permute' the offset of ZST fields arbitrarily, since there is no memory to be accessed? The offset might be visible in other ways (e.g., computing raw pointers and converting them to integers), not sure if that's a concern. I guess the layout of tuples is largely undefined...

Also, just as a point of "curiosity", I think that -- in general -- it's actually plausible for us to reorder DST fields even if they are not last, so long as we can recover not only the alignment of the field but also its size, but it would then require us to do the arithmetic at runtime to compute the offsets of all other fields, something we've traditionally avoided for its complexity.

erikdesjardins · 2020-07-02T15:13:13Z

Yeah, it's possible that the following check could be removed instead:

rust/src/librustc_middle/ty/layout.rs

Lines 393 to 395 in b7856f6

    
           // All other fields must be ZSTs, and we need them to all start at 0. 
        
           let mut zst_offsets = offsets.iter().enumerate().filter(|&(i, _)| fields[i].is_zst()); 
        
           if zst_offsets.all(|(_, o)| o.bytes() == 0) {

but I suspect that other code relies on all fields of a scalar layout having 0 offset so it might be a more complex change.

The relevant parts of layout haven't changed significantly since eddyb's original #45225 so I eagerly await their comments :)

Muirrum · 2020-07-24T00:34:03Z

@eddyb This is a triage bump.

nikomatsakis · 2020-07-27T17:44:02Z

r? @eddyb

Make it official we're waiting for their feedback here=)

eddyb · 2020-08-14T15:45:17Z

Ugh I'm really sorry, I only now got a triage PM about this, I don't recall seeing this PR before (I guess I haven't gotten around to checking the PRs assigned to me in the last 18 days - in general, feel free to PM me on Zulip).

This looks fine, I guess, I'm only worried about the perf loss from sorting twice (which I'm not sure can be avoided).

@bors try @rust-timer queue

rust-timer · 2020-08-14T15:45:19Z

Awaiting bors try build completion

bors · 2020-08-14T15:45:32Z

⌛ Trying commit 7336e5f48c72e3606f8e496daa21890f9415d881 with merge 9ebc0face0731b2ce4ff7382be873c6f734a0c50...

eddyb

Sorry about forgetting about Abi::Vector until the last moment, but it's also important (it also has #[repr(transparent)] support etc.).

You could/should also add tests for it but I'm not sure what type to use (maybe you can find a #[repr(transparent)] test that also tests Abi::Vector using #[repr(simd)] or similar?).

eddyb · 2020-08-18T13:28:51Z

@bors r-

eddyb · 2020-08-18T13:30:16Z

Also, I should say, this shouldn't cause more codegen than today, since any ZSTs that were reordered to the start, still are (that part of the code wasn't changed at all), so they will still get no offset applied.

The new byte-based offset will pretty much only replace an identical offset being applied as a "struct field", but both should result in the same machine code (barring any LLVM optimizations but since this is about ZSTs, I'm not worried about that).

… ZST

RalfJung · 2020-09-05T12:06:57Z

@eddyb this is ready for another round of review.

@erikdesjardins please write a note in the PR when this is ready for review again; GitHub does not (reliably) send email notifications for force pushes.

erikdesjardins · 2020-09-05T16:26:53Z

I messaged @eddyb on Zulip as they mentioned above, but yeah I should probably still comment so other people know what's going on.

erikdesjardins · 2020-09-20T17:51:49Z

@rustbot modify labels: -S-waiting-on-author, +S-waiting-on-review

eddyb · 2020-09-25T13:41:19Z

I got the Zulip PM but then because of how Zulip forces you to always see messages when navigating, it marked it as read, I'm really sorry. I'm still generally months behind most things on GitHub (and even waiting-on-review PRs) but trying to help where I can (don't mind repeated PMs, it helps with various platforms being all over the place).

@bors r+

bors · 2020-09-25T13:41:21Z

📌 Commit 24e0913 has been approved by eddyb

bors · 2020-09-25T14:42:26Z

⌛ Testing commit 24e0913 with merge 5b9e886...

bors · 2020-09-25T16:54:20Z

☀️ Test successful - checks-actions, checks-azure
Approved by: eddyb
Pushing 5b9e886 to master...

rust-highfive assigned petrochenkov Jun 17, 2020

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jun 17, 2020

erikdesjardins commented Jun 17, 2020

View reviewed changes

src/librustc_middle/ty/layout.rs Outdated Show resolved Hide resolved

erikdesjardins commented Jun 17, 2020

View reviewed changes

src/librustc_mir/transform/const_prop.rs Outdated Show resolved Hide resolved

rust-highfive assigned RalfJung and unassigned petrochenkov Jun 17, 2020

rust-highfive assigned nikomatsakis and unassigned RalfJung Jun 18, 2020

erikdesjardins force-pushed the tuplayout branch 2 times, most recently from 083d75a to 1882bdb Compare June 28, 2020 19:47

erikdesjardins commented Jun 28, 2020

View reviewed changes

erikdesjardins force-pushed the tuplayout branch from 1882bdb to 7336e5f Compare June 28, 2020 19:54

erikdesjardins changed the title ~~Allow the last field of sized tuples to participate in field reordering~~ Allow the last field of sized tuples to participate in field reordering (if it's a ZST) Jun 28, 2020

rust-highfive assigned eddyb and unassigned nikomatsakis Jul 27, 2020

Muirrum added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Aug 13, 2020

eddyb suggested changes Aug 18, 2020

View reviewed changes

bors added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Aug 18, 2020

This comment has been minimized.

Sign in to view

erikdesjardins added 6 commits August 30, 2020 14:58

add tests related to tuple scalar layout with ZST in last field

d3b9ece

allow reordering of the last field of a MaybeUnsized struct if it's a…

e5d85f9

… ZST

test that we do not change the offset of ZST tuple fields when unsizing

e9bc3dd

ignore zst offsets instead

68217c9

add codegen test

6fc1d8b

handle vector layout

24e0913

erikdesjardins force-pushed the tuplayout branch from 7b7168b to 24e0913 Compare August 30, 2020 18:58

rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Sep 20, 2020

eddyb approved these changes Sep 25, 2020

View reviewed changes

bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Sep 25, 2020

bors added the merged-by-bors This PR was explicitly merged by bors. label Sep 25, 2020

bors merged commit 5b9e886 into rust-lang:master Sep 25, 2020

rustbot added this to the 1.48.0 milestone Sep 25, 2020

erikdesjardins deleted the tuplayout branch September 25, 2020 23:06

erikdesjardins mentioned this pull request Aug 28, 2023

fix some issues around ZST handling #115277

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Ignore ZST offsets when deciding whether to use Scalar/ScalarPair layout #73453

Ignore ZST offsets when deciding whether to use Scalar/ScalarPair layout #73453

erikdesjardins commented Jun 17, 2020 •

edited

Loading

rust-highfive commented Jun 17, 2020

petrochenkov commented Jun 17, 2020

erikdesjardins commented Jun 17, 2020 •

edited

Loading

RalfJung commented Jun 18, 2020

nikomatsakis commented Jun 18, 2020 •

edited

Loading

erikdesjardins left a comment •

edited

Loading

erikdesjardins Jun 28, 2020

nikomatsakis commented Jul 1, 2020

erikdesjardins commented Jul 2, 2020

Muirrum commented Jul 24, 2020

nikomatsakis commented Jul 27, 2020

eddyb commented Aug 14, 2020

rust-timer commented Aug 14, 2020

bors commented Aug 14, 2020

eddyb left a comment

eddyb commented Aug 18, 2020

eddyb commented Aug 18, 2020

This comment has been minimized.

RalfJung commented Sep 5, 2020

erikdesjardins commented Sep 5, 2020 •

edited

Loading

erikdesjardins commented Sep 20, 2020

eddyb commented Sep 25, 2020

bors commented Sep 25, 2020

bors commented Sep 25, 2020

bors commented Sep 25, 2020

		// This will reorder the last field if it is a ZST, which is okay because
		// there's nothing in memory that could be accessed through an unsized type.

Ignore ZST offsets when deciding whether to use Scalar/ScalarPair layout #73453

Ignore ZST offsets when deciding whether to use Scalar/ScalarPair layout #73453

Conversation

erikdesjardins commented Jun 17, 2020 • edited Loading

rust-highfive commented Jun 17, 2020

petrochenkov commented Jun 17, 2020

erikdesjardins commented Jun 17, 2020 • edited Loading

RalfJung commented Jun 18, 2020

nikomatsakis commented Jun 18, 2020 • edited Loading

erikdesjardins left a comment • edited Loading

Choose a reason for hiding this comment

erikdesjardins Jun 28, 2020

Choose a reason for hiding this comment

nikomatsakis commented Jul 1, 2020

erikdesjardins commented Jul 2, 2020

Muirrum commented Jul 24, 2020

nikomatsakis commented Jul 27, 2020

eddyb commented Aug 14, 2020

rust-timer commented Aug 14, 2020

bors commented Aug 14, 2020

eddyb left a comment

Choose a reason for hiding this comment

eddyb commented Aug 18, 2020

eddyb commented Aug 18, 2020

This comment has been minimized.

RalfJung commented Sep 5, 2020

erikdesjardins commented Sep 5, 2020 • edited Loading

erikdesjardins commented Sep 20, 2020

eddyb commented Sep 25, 2020

bors commented Sep 25, 2020

bors commented Sep 25, 2020

bors commented Sep 25, 2020

erikdesjardins commented Jun 17, 2020 •

edited

Loading

erikdesjardins commented Jun 17, 2020 •

edited

Loading

nikomatsakis commented Jun 18, 2020 •

edited

Loading

erikdesjardins left a comment •

edited

Loading

erikdesjardins commented Sep 5, 2020 •

edited

Loading